HW 2 init #5
HW 2 init #5
Conversation
When working through the code it is often nice to be able to run a single test. Amos King @adkron <[email protected]>
Update Readme to show how to run a single test
I have an issue with `buildMessage` not being exhaustive because it needs to match on an empty map. I really didn't like building all this stuff in the case statement and that is why I pulled it out. I think I need to start thinking of functions returning functions and build it up as arguments get passed in. Amos King @adkron <[email protected]>
src/Cis194/Hw/LogAnalysis.hs
Outdated
|
|
||
| parse :: String -> [LogMessage] | ||
| parse _ = [] | ||
| parse = map (parseMessage) . lines |
src/Cis194/Hw/LogAnalysis.hs
Outdated
| _ -> Unknown s | ||
|
|
||
| buildMessage :: MessageType -> [String] -> LogMessage | ||
| buildMessage m (t:ws) = LogMessage m (read t) (unwords ws) |
There was a problem hiding this comment.
If your function only makes sense for non-empty lists, it can be better to instead take the first element as a distinct argument:
buildMessage :: MessageType -> String -> [String] -> LogMessageand then put the logic for what to do when you don't have one in the caller
"W":w:ws -> buildMessage Warning w ws
src/Cis194/Hw/LogAnalysis.hs
Outdated
| parseMessage s = Unknown s | ||
| parseMessage s = | ||
| case words s of | ||
| "E":i:ws -> buildMessage (Error (read i)) ws |
There was a problem hiding this comment.
read can be OK when you're sure that the input string will parse, or when you're doing some temporary debugging code, but if you want to detect parse failures check out Text.Read.readMaybe :: Read a => String -> Maybe a
| @@ -6,22 +6,44 @@ module Cis194.Hw.LogAnalysis where | |||
| import Cis194.Hw.Log | |||
There was a problem hiding this comment.
A note about a flaw in this exercise: LogMessage shouldn't have an Unknown constructor. Error handling like this should be done in parallel to the type for valid log messages. At a minimum you might have: Either String LogMessage or something. This causes problems later in the exercises when you have to ignore Unknown.
src/Cis194/Hw/LogAnalysis.hs
Outdated
|
|
||
| insert :: LogMessage -> MessageTree -> MessageTree | ||
| insert (Unknown _) t = t |
There was a problem hiding this comment.
As written this pattern is just fine, but a neat thing you can use is record constructor syntax to match a constructor where you don't care about any of its fields. This pays off more for constructors with more fields:
insert Unknown{} t = t
src/Cis194/Hw/LogAnalysis.hs
Outdated
| insert _ t = t | ||
|
|
||
| build :: [LogMessage] -> MessageTree | ||
| build _ = Leaf | ||
| build = foldr insert Leaf . reverse |
There was a problem hiding this comment.
foldr combined with reverse behaves like a foldl with an extra pass.
src/Cis194/Hw/LogAnalysis.hs
Outdated
| whatWentWrong _ = [] | ||
| whatWentWrong = map getString . filter isSever . inOrder . build | ||
|
|
||
| isSever :: LogMessage -> Bool |
src/Cis194/Hw/LogAnalysis.hs
Outdated
|
|
||
| inOrder :: MessageTree -> [LogMessage] | ||
| inOrder _ = [] | ||
| inOrder (Node l m r) = inOrder l ++ [m] ++ inOrder r | ||
| inOrder Leaf = [] |
There was a problem hiding this comment.
This implements the algorithm as specified, but it is somewhat inefficient to left-nest ++ as each nested use must be deconstructed by the next ++. A common pattern for this is:
inOrder :: MessageTree -> [LogMessage]
inOrder t = go t []
where
go (Node l m r) = inOrder l . (m:) . inOrder r
go Leaf = idThis pattern is captured by the dlist package as well as the ShowS type in base.
|
Get it wrong once and autocomplete takes over. Lol
Amos King
Binary Noggin
… On Nov 21, 2018, at 12:26, Eric Mertens ***@***.***> wrote:
@glguy commented on this pull request.
In src/Cis194/Hw/LogAnalysis.hs:
>
-- whatWentWrong takes an unsorted list of LogMessages, and returns a list of the
-- messages corresponding to any errors with a severity of 50 or greater,
-- sorted by timestamp.
whatWentWrong :: [LogMessage] -> [String]
-whatWentWrong _ = []
+whatWentWrong = map getString . filter isSever . inOrder . build
+
+isSever :: LogMessage -> Bool
severe :-)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Whe. I use foldl I get couldn’t match type LogMessage with MessageTree and I wasn’t sure what I did.
Amos King
Binary Noggin
… On Nov 21, 2018, at 12:30, Eric Mertens ***@***.***> wrote:
@glguy commented on this pull request.
In src/Cis194/Hw/LogAnalysis.hs:
>
inOrder :: MessageTree -> [LogMessage]
-inOrder _ = []
+inOrder (Node l m r) = inOrder l ++ [m] ++ inOrder r
+inOrder Leaf = []
This implements the algorithm as specified, but it is somewhat inefficient to left-nest ++ as each nested use must be deconstructed by the next ++. A common pattern for this is:
inOrder :: MessageTree -> [LogMessage]
inOrder t = go t []
where
go (Node l m r) = inOrder l . (m:) . inOrder r
go Leaf = id
This pattern is captured by the dlist package as well as the ShowS type in base.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
I really like the pattern that was used in `inOrder` that was brilliant. Thanks for the help and the pointer on matching a type constructor. Amos King @adkron <[email protected]>
foldl and foldr expect arguments in different orders, but flip allows the arguments to be flipped. Amos King @adkron <[email protected]>
I have an issue with
buildMessagenot being exhaustive because itneeds to match on an empty map. I really didn't like building all this
stuff in the case statement and that is why I pulled it out. I think I
need to start thinking of functions returning functions and build it up
as arguments get passed in.
Amos King @adkron [email protected]